feat: LPT freeze #1840
feat: LPT freeze #1840
Conversation
|
@godexsoft @cindyyan317 This PR is ready for review. There is still a test CI issue around |
| } | ||
|
|
||
| ripple::STAmount | ||
| accountHolds( | ||
| ammAccountHolds( |
There was a problem hiding this comment.
ammAccountHolds and accountHolds seem share some common code , can we refactor them a bit?
There was a problem hiding this comment.
i don't think we benefit much from refactoring to share common code. these two functions should be independent from each other anyways
There was a problem hiding this comment.
My main thought was that pulling out the common code could make things easier to maintain and avoid duplication. If we ever need to update that logic, we'd only have to do it in one place.
For example, the green part is the shared code, if there is bug in it, we need to change two places.
Are these two functions really independent? I saw accountHolds just does one more check comparing with ammAcountHolds. So the former can call latter like this:
ripple::STAmount
accountHolds(
BackendInterface const& backend,
data::AmendmentCenterInterface const& amendmentCenter,
std::uint32_t sequence,
ripple::AccountID const& account,
ripple::Currency const& currency,
ripple::AccountID const& issuer,
bool const zeroIfFrozen,
boost::asio::yield_context yield
)
{
auto amountBeforeCheckingLP = ammAccountHolds(backend, sequence, account, currency, issuer, zeroIfFrozen, yield);
if (!zeroIfFrozen || amountBeforeCheckingLP == amountBeforeCheckingLP.zeroed())
return amountBeforeCheckingLP;
auto const isLptFrozen = [&]() {
if (amendmentCenter.isEnabled(yield, data::Amendments::fixFrozenLPTokenTransfer, sequence)) {
auto const issuerBlob = backend.fetchLedgerObject(ripple::keylet::account(issuer).key, sequence, yield);
if (!issuerBlob)
return true;
ripple::SLE const issuerSle{
ripple::SerialIter{issuerBlob->data(), issuerBlob->size()}, ripple::keylet::account(issuer).key
};
// if the issuer is an amm account, then currency is lptoken, so we will need to check if the
// assets in the pool are frozen as well
if (issuerSle.isFieldPresent(ripple::sfAMMID)) {
auto const ammKeylet = ripple::keylet::amm(issuerSle[ripple::sfAMMID]);
auto const ammBlob = backend.fetchLedgerObject(ammKeylet.key, sequence, yield);
if (!ammBlob)
return true;
ripple::SLE const ammSle{ripple::SerialIter{ammBlob->data(), ammBlob->size()}, ammKeylet.key};
return isLPTokenFrozen(
backend,
sequence,
account,
ammSle[ripple::sfAsset].get<ripple::Issue>(),
ammSle[ripple::sfAsset2].get<ripple::Issue>(),
yield
);
}
}
return false;
};
return isLptFrozen() ? amountBeforeCheckingLP.zeroed() : amountBeforeCheckingLP;
}
`
There was a problem hiding this comment.
truthfully, the code in ammAccountHolds:
if (ripple::isXRP(currency))
return {xrpLiquid(backend, sequence, account, yield)};
should be replaced with an assertion ASSERT(!ripple::isXRP(currency)), which is why I think these two functions are independent
There was a problem hiding this comment.
If you genuinely feel certain modifications are necessary, please go ahead and adjust the code accordingly. IMO, refactoring common logic doesn’t mean you can’t make case-specific tweaks; it just helps keep the codebase cleaner and easier to maintain.
There was a problem hiding this comment.
they may share some common piece of code now, but that may not be the case soon. In fact i believe the implementation ammAccountHolds will soon diverge from accountHolds due to the DeepFreeze feature. For the incoming deepfreeze feature,accountHolds will need to add isDeepFreeze check (that's not gated by an amendment).
There was a problem hiding this comment.
accountHolds will need to add isDeepFreeze check
Would it make it impossible for accountHolds calls ammAccountHolds?
Since you prefer to keep it as is, I'll respect your decision.
There was a problem hiding this comment.
I'd prefer leaving as it as for now. In the future there some more incoming amendments that will change this function and two functions will look completely different
There was a problem hiding this comment.
but i'd be open to make this change later if you think it is still necessary.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1840 +/- ##
===========================================
+ Coverage 72.71% 72.99% +0.27%
===========================================
Files 333 335 +2
Lines 13525 13751 +226
Branches 6881 6983 +102
===========================================
+ Hits 9835 10037 +202
- Misses 1785 1796 +11
- Partials 1905 1918 +13 ☔ View full report in Codecov by Sentry. |
tests/common/util/TestObject.cpp
Outdated
| @@ -260,6 +261,10 @@ createAccountRootObject( | |||
| accountRoot.setFieldH256(ripple::sfPreviousTxnID, ripple::uint256{previousTxnID}); | |||
| accountRoot.setFieldU32(ripple::sfPreviousTxnLgrSeq, previousTxnSeq); | |||
| accountRoot.setFieldU32(ripple::sfTransferRate, transferRate); | |||
|
|
|||
| if (ammID != ripple::uint256{0}) | |||
There was a problem hiding this comment.
Do we not have a constant for this zero somewhere? Consider adding if not
There was a problem hiding this comment.
i've actually changed it to optional which i think makes more sense
tests/unit/rpc/RPCHelpersTests.cpp
Outdated
| ON_CALL(*mockAmendmentCenterPtr_, isEnabled(testing::_, Amendments::fixFrozenLPTokenTransfer, testing::_)) | ||
| .WillByDefault(testing::Return(true)); | ||
|
|
||
| auto const ammID = ripple::uint256{54321}; |
There was a problem hiding this comment.
We usually add constants like these at the top of the file in an anonymous namespace and make them constexpr
XRPLF/rippled#5227
Introduces amendment gating in
accountHoldsthat so that the assets in the AMM are not frozen